Skip to content

Add codespell: config + action (to detect new typos) and make it fix a good number of them #11195

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

yarikoptic
Copy link
Contributor

@yarikoptic yarikoptic commented Mar 7, 2024

codespell was used occasionally on some files, so not a new tool here. But now it would guard RTD from having typos introduced to begin with.

Note that some fixes could affect (fix) functionality.

There is an odd "variable" syntaxt which I didn't fix since seems would require transition and smells like it is on purpose:

❯ git grep syntaxt -- readthedocs/
readthedocs/projects/migrations/0106_add_addons_config.py:                ("syntaxt", models.CharField(max_length=128)),
readthedocs/projects/models.py:    syntaxt = models.CharField(max_length=128)
readthedocs/projects/tasks/builds.py:        grab them by using glob syntaxt between other files that could be garbage.

or was it intended to be syntax ?

TODOs


📚 Documentation previews 📚

@yarikoptic yarikoptic requested review from a team as code owners March 7, 2024 23:19
Copy link

sentry-io bot commented Mar 7, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: readthedocs/api/v2/views/integrations.py

Function Unhandled Issue
get_closed_external_version_response TypeError: 'NoneType' object is not subscriptable /api/v2/webhook/{project_slug}/{integration_pk}...
Event Count: 1

Did you find this useful? React with a 👍 or 👎

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR 👍🏼

I didn't take a full review, but I noted a few issues that I see will generate some conflicts. In particular, those that change Python code and require some manual interaction (e.g. a Django migration to update the help_text)

I'm not opposed to this work, but I find it hard to prioritize by the core team at this point.

@agjohnson
Copy link
Contributor

We use pre-commit for all other linting checks. I feel if we implement this it should be at pre-commit, not GHA.

@yarikoptic
Copy link
Contributor Author

Hmm, interesting, my script should've detected presence of pre-commit config... Will have a look wherever get a chance. You do have ci job running the pre-commit I assume? (I don't see report from pre-commit service)

@yarikoptic
Copy link
Contributor Author

that explains it:

❯ ls -ld .pre-commit-config.yaml
lrwxrwxrwx 1 yoh yoh 29 Mar  7 18:00 .pre-commit-config.yaml -> common/pre-commit-config.yaml
❯ cat .pre-commit-config.yaml
cat: .pre-commit-config.yaml: No such file or directory
❯ git submodule
-4af0fffd2cbeeb40f0a71b875beb99d6dc88a355 common

Let's do the submodules dance now...

@humitos
Copy link
Member

humitos commented Jul 24, 2025

This PR is great! Thanks for opening it!

I'd like to move forward with it. I've already reviewed readthedocs/common#212

We need to make some adjustments here and we can move forward 👍🏼 . @yarikoptic are you able to continue with this work?

Final decision on fixing or not CHANGELOG.rst

Let's exclude the changelog for now. It has a lot of noise.

let me exclude all such lines with help_text *=.*

We should implement this, if it's not already.

@yarikoptic
Copy link
Contributor Author

We need to make some adjustments here and we can move forward 👍🏼 . @yarikoptic are you able to continue with this work?

I am able as soon as there is cycles on your end so PR doesn't amass conflicts to address again.

Copy link

read-the-docs-community bot commented Jul 24, 2025

Documentation build overview

📚 dev | 🛠️ build #28967931 (c5202bd) | 🔍 preview

Files changed

Comparing with latest (000af14...c5202bd)

Show files (5) | 5 modified | 0 added | 0 deleted
File Status
aws-temporary-credentials.html 📝 modified
github-app.html 📝 modified
design/build-images.html 📝 modified
design/future-builder.html 📝 modified
design/new-search-api.html 📝 modified

Copy link

read-the-docs-community bot commented Jul 24, 2025

Documentation build overview

📚 docs | 🛠️ build #28967932 (c5202bd) | 🔍 preview

Files changed

Comparing with latest (000af14...c5202bd)

Show files (1) | 1 modified | 0 added | 0 deleted
File Status
guides/creating-project-private-repository.html 📝 modified

@yarikoptic
Copy link
Contributor Author

FWIW, I have rebased and redone codespell fixing (automated and interactive). Let's see if no side effects. Please review the diff.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! I suppose there may be a few test failing because there are some words that shouldn't return search results.

@@ -526,7 +526,7 @@ def test_search_advanced_query_detection(self, api_client):
search_params = {
"project": project.slug,
"version": version.slug,
"q": "indx",
"q": "index",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was probably an invalid word on purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we need to annotate this line to be skipped and ok to contain indx?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced it with explicit "abracadabra" , or it does have to be "indx"? (let's see if tests feel better)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is failing. I think it needs to be indx probably for fuzzy search testing. I would leave as it was to avoid this type of issues and probably mark that line to be skipped 👍🏼

…nteractively

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "codespell -w -i 3 -C 4",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
…os automagically

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "codespell -w",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
yarikoptic added a commit to yarikoptic/common that referenced this pull request Jul 24, 2025
@humitos
Copy link
Member

humitos commented Jul 25, 2025

There is an odd "variable" syntaxt which I didn't fix since seems would require transition and smells like it is on purpose:

This is indeed a typo that I made when I created the model's field AddonSearchFilter.syntaxt, yes 😓 . Let's leave as-is for now because it will require a migration. We can do this in a different PR. It should be easy since this field it's not being used yet.

Comment on lines +48 to +56
[codespell]
# Ref: https://github.com/codespell-project/codespell#using-a-config-file
skip = .git,*.svg,locale,package-lock.json,*.css,*.min.*,vendor,*.ai,setup.cfg,migrations,CHANGELOG.rst,common
check-hidden = true
# some names and abbreviations and very long lines (minimized?)
# TODO: fixup help_text in readthedocs/builds/models.py : "to perfom an"
ignore-regex = \b(Manuel|DED|Wile E. Coyote|Couldn\\u2019t|to perfom an)\b|.{300,}|"pyton\b|\|(ative|ment)\||"Hel" will match\b|ative: ''|help_text *=.*
# TODO: fix syntaxt -- would require transition?
ignore-words-list = fo,te,astroid,requestor,syntaxt,ore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to put all this configuration on a specific codespell file? Like .codespell.cfg or similar?

We will need to put this file into common/ repository (readthedocs/common#212) because we want to share this configuration across multiple repositories, eg. https://github.com/readthedocs/ext-theme

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems .codespellrc https://github.com/codespell-project/codespell#using-a-config-file

We want that file to be in common/ and symlinked from here 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants